-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Xhr data #9972
Xhr data #9972
Conversation
Why is |
Based on the discussion on #8807 and looking at the spec, when the response is an error the status message is empty and not 'OK' like the test expects. As for the comparison for 'Content-Type', servo seems to return the string 'text/html; charset=utf-8', with the space, which doesn't quite match the original expected value. |
@@ -23,14 +23,14 @@ | |||
if (method.toUpperCase() !== 'GET') { | |||
assert_equals(client.status, 0); | |||
assert_equals(client.responseText, ''); | |||
assert_equals(client.statusText, 'OK'); | |||
assert_equals(client.statusText, ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically, why is this modified from OK to an empty string?
My concern here is whether Firefox and other browsers pass this test as-is without your changes. We shouldn't modify it unless the test itself is wrong. In which case, a PR to the upstream repository at https://github.com/w3c/web-platform-tests should be made. |
Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions. tests/wpt/web-platform-tests/XMLHttpRequest/data-uri.htm, line 26 [r2] (raw file): tests/wpt/web-platform-tests/XMLHttpRequest/data-uri.htm, line 46 [r2] (raw file): Comments from the review on Reviewable.io |
Regarding opening a PR with https://github.com/w3c/web-platform-tests, should I revert the changes here (to the test) and wait for that PR to land before proceeding with this one? |
Yes, if the tests are wrong, open a PR at w3c/web-platform-tests and wait until that PR is merged. |
Thanks, the PR is here: web-platform-tests/wpt#2667. I will update once it's merged and the servo copy is synced. |
Still waiting on an update of our local WPT clone at this point. |
☔ The latest upstream changes (presumably #10315) made this pull request unmergeable. Please resolve the merge conflicts. |
It merged! |
This was intended to fix servo#8015 but the tests are all still failing as of this commit.
@bors-servo r+ Thanks! |
📌 Commit 6f2bce7 has been approved by |
Xhr data Builds on existing work by @emosenkis. Fixes #8015. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9972) <!-- Reviewable:end -->
☀️ Test successful - android, arm32, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor |
Builds on existing work by @emosenkis. Fixes #8015.
This change is